Skip to content

sbt-dotty: Add a setting to exclude a configuration from the Dotty IDE, and exclude the optimized projects from the Dotty build #3255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 5, 2017

Conversation

smarter
Copy link
Member

@smarter smarter commented Oct 4, 2017

No description provided.

@smarter smarter requested a review from Duhemm October 4, 2017 18:57
@@ -15,6 +15,16 @@ import scala.collection.mutable.ListBuffer
import DottyPlugin.autoImport._

object DottyIDEPlugin extends AutoPlugin {
object autoImport {
val excludeFromIDE = settingKey[Boolean]("If true, do not import this project or configuration in the Dotty IDE")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's a good idea, is it worth considering naming this key ide-skip-project? To match https://github.com/JetBrains/sbt-ide-settings/blob/750b993453fb3d1f31f371968d06e3fc792870a1/src/main/scala/sbtide/Keys.scala#L11-L12

Also, by convention, keys from the same plugin should ideally use the same prefix:

  • ideLaunch
  • ideExclude
  • ideCommand
  • ideRun

This convention makes it easy to explore available plugin keys from the sbt shell.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth considering naming this key ide-skip-project?

Using the same key name as another plugin seems like a bad idea in fact, because then they will conflict with each other, I have no idea what that does in the sbt repl but in build files you'll get errors due to ambiguity.

Also, by convention, keys from the same plugin should ideally use the same prefix

Maybe yes, feel free to open an issue for that. I have no idea what the good practices for sbt plugins are :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of defining a new key, can't we just use skip in launchIDE := true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't be accurate since launchIDE just calls configureIDE and I don't think we can do skip in configureIDE since configureIDE is a command and not a task. It also wouldn't be complete since we also want to disable the config for compileForIDE. All in all, it seems better to have a specific key for this purpose :).

Copy link
Contributor

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment / question / suggestion. Feel free to merge if you dont think it's important.

LGTM!

// we only need to import one set in the IDE. We prefer to import the
// non-optimized projects because optimize is slower to compile and we do
// not trust its output yet.
excludeFromIDE := true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the above comment as "*-bootstrapped projects must be skipped, so why isn't this in commonBootstrappedSettings instead? My guess is that it's because dotty-language-server exists only in bootstrapped-mode and shouldn't be skipped?

Interestingly, commonBootstrappedSettings contains:

EclipseKeys.skipProject := true

It would probably be less surprising if kept close to excludeFromIDE := true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are three kinds of projects with the same set of sources:

  • Non-bootstrapped (compiled with Scala 2.12)
  • Bootstrapped (compiled with Dotty non-bootstrapped, default options)
  • Optimized (compiled with Dotty non-bootstrapped, with -optimise added)

For the Dotty IDE, the non-bootstrapped don't matter: they're not Dotty-compiled projects so they will be skipped by the plugin, so we only need to decide if we want the bootstrapped or the optimized one, we prefer the bootstrapped non-optimized for the reason given by the comment.

For Eclipse, the situation is different: the only projects which we want to import are the non-bootstrapped one, because the Scala IDE can only deal with Scala 2-compiled projects, so we skip everything else (commonBootstrappedSettings is also part of the settings used for optimized projects).

Hope this clears things up :).

@smarter smarter merged commit af528a1 into scala:master Oct 5, 2017
@allanrenucci allanrenucci deleted the exclude-from-ide branch December 14, 2017 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants